-
Notifications
You must be signed in to change notification settings - Fork 7.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Working browserify. Not backwards compatible. #1353
Conversation
I'm going to close the PR so that it doesn't accidentally get merged in. |
Nice man, that's a ton of work. It seems like the challenge here is we're not just publishing a single module, we're publishing an ecosystem of modules that we want to be useable in the browser, post-compile. What if every internal component had a dual export?
It's kind of the same as having a main file that pulls in all modules.
The latter doesn't seem like a terrible way to do it either. Just less automatic. |
The problem with that is that we explicitly do not want to export to |
I did some cleanup. Removed all the, now, unnecessary
Afterwards, I'll spend more time to see if I can make it backwards compatible (though, I don't think it can be done easily and well), so, we'd need to come up with a new way to register components and techs. I think probably the best way is to follow an approach similar to plugins, where we have a method on videojs that allows you register a component or tech. Probably best to have separate mechanisms for techs and for components. And finally, I'd need to address the tests. |
This is exciting and not small. Want to set up a hangout to talk through this? |
Component.prototype.addChild = function(child, options){ | ||
var component, componentClass, componentName, componentId, components; | ||
|
||
components = require('./components.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is a different file that includes all components. Not requiring itself. :-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, though, components.Component
is itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this again, there's a circular dependency here between component.js and components.js. It seems we've just moved the issue up the chain. Would it help if every component registered itself like components.registerComponent('Component', Component)
instead exporting them in components.js? And at the same time if components and component were the same file?
I'd like to come up with a cleaner naming convention then vjslib and vjsevents. Any ideas around that? |
|
sure, that works for me. I'm sure things might change again if we bring in lodash. |
I know some of the code that uses events has variables that are called And yeah, I'd love to break down |
Yeah, the tech mechanism could just be something that registers a component and then also adds it to the techOrder. Maybe This doesn't currently seem to handle the custom component use case, e.g.
Which I know having a way to register components will help with, but that's a good API workflow to keep in mind as we're designing this system. And it doesn't have to look like that example if there's a better way, but it should be clean. Two questions need to be answered from that example.
In the current implementation, without any magic, it looks like it would be:
|
What about maybe: MyButton = Button.extend({});
videojs('my_id', { children: { 'myButton': MyButton } }); I.e., if the object is a function, assume it's the constructor function for creating that object. But really, I'm not a fan of implicit things like this because in this case MyButton gets added globally and then its assumed that videojs will be able to access that global property.
videojs.registerComponent = function(name, component) {
require('./components.js')[name] = component;
} And then to init a component and add it as a child: var foo = function Foo() {} // just some thing
var videojs = require('video.js');
videojs.registerComponent('myfoo', foo);
var player = videojs("home_video", {
children: {
myfoo: {}
}
});
var Button = require('video.js/button'); But otherwise, we'd need to export it either as it is now ( |
Do you consider anything added to the videojs object 'global'? One thing to keep in mind here is this is currently a UI library as much as it is a video player, the goal being to allow people to build custom UI elements for the player. Once it's compiled everything needs to be exposed somehow (globally?), as much as any function on lodash needs to be accessible from _.functionName. Does that makes sense and do you agree with that? |
Adding to the videojs object isn't necessarily global, but currently, the way videojs works, if components are attached to videojs directly rather than via a function, would create cyclical dependencies that break things. We definitely should strive for the best/easiest interface while also not painting ourselves into a corner. Exposing properties through the In that specific example, videojs.MyButton = Button.extend({});
videojs('my_id', { children: { 'myButton':{} } }); |
Mentioned this in chat but recording it here too: A big issue with making video.js modular like you'd like is Flash. We have to pass Flash functions that are in global scope for it to fire events and errors. https://github.com/videojs/video.js/blob/v4.6.4/src/js/media/flash.js#L39 YouTube works the same way. https://developers.google.com/youtube/js_api_reference I guess it could be possible to make the functions player specific. Dynamically creating their names based on the player or swf ID. But they still need to be in global scope for flash to be able to trigger them. Maybe that's good enough? |
Yep, that's what I mentioned as well. While we have to leak some stuff globally because of flash, we should minimize that as much as possible and even, as you mentioned, have a flash specific method per player/swf object. |
It looks like testing a browserify-based project in the browser requires a bundle step first. Is that right? If so that seems like it could make debugging a test error more of a pain. |
Yes, but you can use watchify to have it autobuild and what not. |
Would it be possible to use source maps or something to connect line numbers to the original source? |
Yes, definitely. If you run browserify/watchify with --debug, it'll have sourcemaps. |
There’s nothing we could do for IE8 though, right? |
es5-shim. |
I can’t tell if that answers my question about sourcemaps in IE8. On another note. What do you think about cis-umd as an option for requiring videojs but not bundling it? |
Oh, sorry. Yes, sourcemaps aren't available in IE8. So, for that you'd want to just not minify. cjs-umd or whatever other thing is ok but it only maybe solves the outside api. It's missing the possibly more important piece of having the internals of videojs be modularized themselves. |
Gradually wrapping my brain around the whole problem set here. There's two end-developer workflows that we need to have stories for.
At this point everything is built around the script includes workflow, and we add UMD to provide a module interface for video.js. We're in the process of shifting over to where everything is built around the modules workflow, and we need stories around what that looks like for external plugins, techs, and shared components, while maintaining support for the script includes workflow. There's a few stages to this that I can see.
I think this PR is close to the end of 2. I don't have a clear view yet of what 3/3.5 will look like but I think understanding that could influence earlier steps. Can you see that far ahead? |
I have some ideas for 3. I'll write them up soon when I have some time. Hopefully by the end of this week. |
This UMD-jQueryPlugin example looks like it could be a good starting point. |
One issue that @heff and I were just brainstorming is how to get around the disparity between what's in our code base and plugin authors / users will potentially see (particularly if they don't go the Browserify route). For instance, a dev that's just doing everything on This does buy us some additional niceties as well:
I think this is most of what we talked about...@heff am I missing anything? |
I think that covers it. A plugin could then look like...
I like it because it can create a 1 to 1 mapping between our internal dependencies and what's available to plugin authors, instead of creating a separate ways to export components compared to other types of dependencies. |
A dev shouldn't need to know look at our code to use it. That's what the API and docs are for. I don't think that it should be called var Button = videojs.getComponent('button'); Or something which would be mirrored by a I don't think we should expose dependencies directly. Though, if a user were using browserify, they would be able to do var Button = require('video.js/components/button'); The jquery-umd plugin example is exactly what it should look like. But again, a plugin author doesn't have to target everywhere video.js works, though, it would be nice. Updating the plugin generator with this would be great. Especially with the addition of a build tool that would just auto-wrap the plugin with the appropriate header/footer. If everyone just used browserify, our lives would be much easier :) |
While I agree in principal that reading in the code shouldn't be necessary, you can look through issues, plugins, etc and see that's simply not the case in practice. A lot of that is
The point of exposing dependencies directly like this would be for folks not using Browserify. Either I'm terrible at reading, or you added things after I started typing, because that last line pretty much wraps everything I just said in a nutshell. |
I didn't add anything. I agreed with the general idea but didn't agree on the specifics. |
@gkatsev can you go into more detail on why you think exposing dependencies directly is a bad idea? |
because then our users will depend on our dependencies and then we will not be able to switch dependencies and that is the last thing we want to happen. |
I disagree, I think this actually helps us in that regard. With this work flow we can give reasonable deprecation warnings and things along those lines as opposed to people using them anyway and us just giving them a ¯_(ツ)_/¯ when we break things. |
I don't think users should be relying on our dependencies. If they want lodash, they should include lodash. We shouldn't need a deprecation warning if internally we switched from lodash to underscore various utilities. |
We should expose some methods, like |
I agree with @gkatsev that we shouldn't make it the default that any video.js external dependencies are available that way for plugin devs. We don't want to lock ourselves into including specific external libs, especially for minor functionality. I'm on the fence with lodash specifically, whether we use 'videojs.require' or not, since it's so universal. Either way we have a few other internal classes/libraries that could also be made available this way that don't fall under 'components' specifically. Starting with CoreObject (which is essentially Class). There's also been a few cases now where I've wanted an EventListener class that's between CoreObject and Component. Beyond that there's the fullscreen API and a host of additional utility functions that could potentially be organized this way. The option for all of these is either to 'globalize' them onto videojs (or a nested property), or register and load them dynamically through something like I'm liking the registration method because it allows us to more cleanly deprecate things like Matt said. I'm not sure how far that should go, but even with utility functions it's kind of a nice idea that it would take the main videojs object away from being another utility library, e.g. I'm personally liking the
That would be cool, and we should definitely make it possible, but I don't think we'll be able to write plugins this way and have it still support the 'Script Includes' workflow. At least I can't think of a clean way where the plugin would still share the submodules with core at run time. |
See gkatsev#4 for some additional work on this. I think we could move the discussion around how to expose internal modules to plugin developers to its own issue. We did have some additional interesting thoughts on that, like allowing plugins to state which other plugins it relies on, including versions. |
We should handle this first before any of the other 5.0 tasks. gkatsev#4 still needs to be merged, and then there will probably be a bunch of merge conflicts. Might even be easier to use this as a guide and start from scratch. @gkatsev do you think you'll have time to address this soon or should we open it to whoever can get to it? |
I think we need to decide on #1812 before going any further. |
Closing in lieu of #1976, which builds on this + ES6. |
DO NOT MERGE.
This is the work-in-progress for browserifying videojs. The
bundle.js
file is built usingfrom within the videojs directory. Can also just run
npm run build
.This will create a
bundle.js
file which is a standalone (UMD) build of videojs. You can then load the bundle file directly in the browser together with the generated CSS and you have a working videojs player.You can also consume videojs directly from another app/module that uses browserify by just
require('video.js')
and then when you run browserify it'll figure things out.Unfortunately, this isn't backwards compatible right now. The various components aren't really exposed directly right now and if you add more components it won't find them on the videojs object since it excepts them on the
./components.js
module. Though, more modules could be added by requiring the components.js module (require('video.js/components
)` and then adding the extra component to it.Probably a better way is to create a function for registering components and techs.
I'm going to see whether I can make it backwards compatible. Also, this needs to be cleaned up because we don't need all the
var vjs = {}; /* ... */ module.exports = vjs
everywhere. That var is superfluous.